Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NewClient() can't handle credentials containing characters such as / #39

Merged
merged 3 commits into from
Sep 5, 2017

Conversation

opalmer
Copy link
Contributor

@opalmer opalmer commented Sep 4, 2017

See commit ba033b0 for an explanation.

…haracters

Depending on the contents of the username and password the default
url.Parse may not work. The below is an example URL that
would end up being parsed incorrectly with url.Parse:
  http://admin:ZOSOKjgV/kgEkN0bzPJp+oGeJLqpXykqWFJpon/Ckg@localhost:38607

So instead of depending on url.Parse we'll try using a regular expression
first to match a specific pattern instead. This won't be 100% reliable
in all cases which is why we fall back onto using url.Parse anyway.
@opalmer opalmer added the bug label Sep 4, 2017
@opalmer opalmer added this to the 0.4.0 milestone Sep 4, 2017
@opalmer
Copy link
Contributor Author

opalmer commented Sep 4, 2017

@andygrunwald, please take a look. Discovered this while working on gerrittest which connects to a container and asks Gerrit to generate a password (which in turns breaks NewClient() because of the '/' character).

Once this merges we probably should go ahead and release 0.4.0.

EDIT
For reference, here's the failing test output I got: https://gist.github.com/opalmer/4d381e03d2f13ae8f47c7395b7105f3a

And the test generating the failure is here: https://github.com/opalmer/gerrittest/blob/f85732e99ad39a58b0e5ef2913fb6e4bbc87792a/cmd/subcommands/integration_test.go#L50

Basically this is:

  • Starting a docker container
  • Hitting /login/ to generate a cookie.
  • Using that cookie to ask Gerrit to generate a password.
  • Using the generated password to connect to Gerrit using go-gerrit (which fails because the password contains /)

@codecov-io
Copy link

codecov-io commented Sep 4, 2017

Codecov Report

Merging #39 into master will increase coverage by 0.14%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #39      +/-   ##
==========================================
+ Coverage   17.79%   17.94%   +0.14%     
==========================================
  Files          21       21              
  Lines        1742     1750       +8     
==========================================
+ Hits          310      314       +4     
- Misses       1398     1403       +5     
+ Partials       34       33       -1
Impacted Files Coverage Δ
gerrit.go 80.75% <100%> (-1.21%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 179c3f3...28a9265. Read the comment docs.

@andygrunwald andygrunwald merged commit b63100d into master Sep 5, 2017
@andygrunwald andygrunwald deleted the fix-url-auth-parsing branch September 5, 2017 05:29
@andygrunwald
Copy link
Owner

Another good one!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants